refactor: add storage trait abstraction for ValidatorDB and unit tests#111
refactor: add storage trait abstraction for ValidatorDB and unit tests#111
Conversation
|
This PR currently has no labels. Based on the diff and description, the The PR adds new storage trait abstractions ( Of the available labels, |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn temp_db() -> ValidatorDB { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let db = ValidatorDB::new(dir.path().join("test.redb")).unwrap(); | ||
| std::mem::forget(dir); // OS cleans up when process exits |
There was a problem hiding this comment.
std::mem::forget is the wrong tool here — it leaks the TempDir and never cleans it up (even though it works by accident, since the directory outlives the test). The idiomatic fix is to return the TempDir from the helper so its drop runs when the test ends:
| std::mem::forget(dir); // OS cleans up when process exits | |
| let dir = tempfile::tempdir().unwrap(); | |
| let db = ValidatorDB::new(dir.path().join("test.redb")).unwrap(); | |
| (db, dir) |
Change the return type to (ValidatorDB, tempfile::TempDir) and update each call site to let (db, _dir) = temp_db();.
| let headers: Vec<_> = tasks.iter().map(|(block, _, _)| block.header.clone()).collect(); | ||
| db.grow_remote_chain(&headers)?; |
There was a problem hiding this comment.
The &[Header] trait signature forces an allocation + full clone of every header on the hot path. The original code passed an iterator with no allocation:
// before
db.grow_remote_chain(tasks.iter().map(|(block, _, _)| &block.header))?;Consider changing the trait method to accept impl IntoIterator<Item = &Header> — though that isn't directly expressible in a trait method today without a generic parameter. A practical middle ground: keep &[Header] in the trait but change the inherent ValidatorDB::grow_remote_chain to also accept &[Header] so the trait impl is a zero-cost pass-through, and fix the call site to avoid the clone by collecting &block.header into a Vec<&Header>:
| let headers: Vec<_> = tasks.iter().map(|(block, _, _)| block.header.clone()).collect(); | |
| db.grow_remote_chain(&headers)?; | |
| let headers: Vec<&Header> = tasks.iter().map(|(block, _, _)| &block.header).collect(); | |
| db.grow_remote_chain(&headers)?; |
This still needs a Vec but at least avoids cloning each Header.
| db.rollback_chain(52).unwrap(); | ||
| assert_eq!(db.get_local_tip().unwrap().unwrap().0, 52); | ||
| // Remote chain should also be rolled back | ||
| assert!(db.get_remote_tip().unwrap().is_none()); |
There was a problem hiding this comment.
This assertion doesn't test what the comment claims. All 5 blocks were promoted to canonical before rollback_chain is called, so the remote chain is already empty at that point — the assertion is vacuously true regardless of what rollback_chain does to the remote chain.
To actually test that rollback clears remote-chain state, leave some blocks unpromotod and verify they're gone after the rollback. For example, only promote 3 of the 5 blocks and then roll back to a point that includes the unpromoted range.
|
Good direction — the trait abstractions are clean and the delegation impls are correct. Three issues to address:
|
1 participant
Summary
Introduce storage trait abstractions (
ChainState,TaskQueue,BlockDataStore,ValidationResultStore) that decouple consumers from the concreteredb-backedValidatorDBimplementation, enabling alternative backends (e.g., in-memory for testing) without changing consumer code.storage_traits.rswith four domain-specific traits covering chain state management, task queue lifecycle, block/witness data retrieval, and validation result storageValidatorDBvia delegation to existing inherent methods (purely additive, no behavior change)chain_sync.rsto use trait bounds (DB: ChainState + TaskQueue) instead of concrete&ValidatorDB/Arc<ValidatorDB>parametersValidatorDBunit tests covering chain growth & promotion, rollback, genesis round-trip, anchor block, contract code cache, block hash lookup, earliest local block, and history pruningtempfiledev-dependency for test database isolationDetails
Storage Traits (
storage_traits.rs)ChainStateTaskQueueBlockDataStoreValidationResultStorechain_sync.rsGeneralizationfetch_blocks_batch,remote_chain_tracker, andfind_divergence_pointnow accept genericDBparameters bounded by the appropriate traits. Call sites in binaries continue to passValidatorDBunchanged since it implements all traits.Unit Tests (8 tests)
chain_growth_and_promotion— anchor + remote growth + promote to canonicalrollback— grow chain then roll back to earlier blockgenesis_round_trip— store and reload genesis configanchor_block_round_trip— reset and retrieve anchor blockcontract_code_cache— add and query contract bytecodesget_block_hash_canonical_and_remote— hash lookup across both chainsearliest_local_block— empty chain and post-anchor queriesprune_history— prune old canonical entriesTest plan
cargo check --workspacepassescargo test -p stateless-corepasses (includes 8 new tests)cargo clippy --workspace --all-targets --all-featuresclean